Add Patch to correct Profiler output to not report negative numbers a… #37
Conversation
leandro-lucarella-sociomantic
left a comment
leandro-lucarella-sociomantic
left a comment
There was a problem hiding this comment.
Also, try to make the commit title ~50 chars long and add a more detailed description of what's going on in the commit body.
| - fprintf(fplog,"%7llu%12lld%12lld%12lld %.*s\n", | ||
| + fprintf(fplog,"%llu %llu %llu %llu %.*s\n", | ||
| calls, tl, fl, pl, id.length, id.ptr); | ||
| } |
There was a problem hiding this comment.
This is doing much more than what the title says, you should clarify why timings are being removed and why negative values were reported before.
There was a problem hiding this comment.
%lld -> print as signed number, %llu print as unsigned.
there is no timing removed the line that I removed was never used.
There was a problem hiding this comment.
OK, this is why the commit needs a better description, it is very hard to understand what is going on and what's your intention here. You are indeed removing the freq from this. You are also removing the space that makes the output looks like a table, now the header and the values won't be aligned anymore. Why did you do that?
I think having times in usec is useful to get a better grasp of how much real time is being spent. Why are you converting that into ticks? I still don't understand where is the issue you are trying to fix.
There was a problem hiding this comment.
ah the timing is not actually in usecs.
it's just ticks divded by a fixed constant.
Under windows it actually fetches a real processor frequency.
But under linux the frequency is hard-coded to 3 mhz.
Since it's hard to get the freq under linux I decided to go with ticks.
There was a problem hiding this comment.
All that should be explained in the commit message.
stefan-koch-sociomantic
force-pushed
the
profiler_fix
branch
2 times, most recently
from
93a52ec to
c7cddaf
Compare
|
Updated. |
leandro-lucarella-sociomantic
left a comment
leandro-lucarella-sociomantic
left a comment
There was a problem hiding this comment.
Recap:
I see 3 different changes here, and every change should go in a separate patch/commit.
-
The "negative" issue.
I still don't understand why it could happen before and how it is fixed now. If it is because you changed a
longtoulong, then aren't you just hiding the problem instead of fixing it? All the answers to this questions should go to the commit message. -
The
freqis not used anymore to display the results.You should explain in the commit message why using the
freqto display the results doesn't make sense (what you said about being hardcoded in Linux, etc.). I would favour showing both the time as ticks and as usec, and just print an extra big warning in Linux saying the Freq is hardcoded to whatever. -
Changes in the output format.
The new format shouldn't be unreadable, some way to show the results in an understandable way should be found. That commit message should show how the format was before and after the fix.
| - fprintf(fplog,"%7llu%12lld%12lld%12lld %.*s\n", | ||
| + fprintf(fplog,"%llu %llu %llu %llu %.*s\n", | ||
| calls, tl, fl, pl, id.length, id.ptr); | ||
| } |
There was a problem hiding this comment.
All that should be explained in the commit message.
|
About "Uhe printing is corrected from using %lld to using %llu to avoid seeing negative numbers in cases where we'd overflow" (besides the typo "Uhe"), shouldn't overflow be noticed somehow? Otherwise the information you get won't be negative but it will make no sense either, and there is no way to tell it doesn't make sense. |
stefan-koch-sociomantic
force-pushed
the
profiler_fix
branch
4 times, most recently
from
2b2ce92 to
cb0c09d
Compare
|
@leandro-lucarella-sociomantic ready for a second review. |
stefan-koch-sociomantic
force-pushed
the
profiler_fix
branch
from
cb0c09d to
603668c
Compare
This patch fixes the profiler-outptut form looking like this
```
5078127277490292274277443610704 54635 int ocean.sys.Epoll.Epoll.wait(ocean.sys.Epoll.epoll_event_t[], int)
```
to looking like this
```
16384 680160372 680160372 41513 void test_prof.f2()
```
Because it is hard to get the cpu frequency under linux,
the profiler uses a hard-coded frequency (3mhz) when running linux.
Thereby introducing misleading information into the perforamnce-profile.
We are circumventing the problem by printing cycles instead of us,
and fix the ouput to include the necessary left-padding for the larger figures.
stefan-koch-sociomantic
force-pushed
the
profiler_fix
branch
from
603668c to
5e36c73
Compare
Can you please split the commits as requested in #37 (review)? It shouldn't be that complicated, just use git to create the different commits with the base tag dmd transitional is based on and then export them using |
|
Also it would be good if you describe what changed in the new round, as sadly the |
|
@leandro-lucarella-sociomantic the negative numbers where an artefact of printf formatting the number. |
|
OK, my other comments/change requests are still pending though. |
3 participants
…s well as always put a space between diffrent values